src/openhuman/channels/commands.rs (1)

307-308: Simplify redundant default initialization in test setup.

Config::default() already provides default channels_config, so the explicit reassignment can be removed.

Proposed simplification

-        let mut config = Config::default();
-        config.channels_config = crate::openhuman::config::ChannelsConfig::default();
+        let config = Config::default();
         doctor_channels(config).await.unwrap();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/channels/commands.rs` around lines 307 - 308, The test creates
a Config via Config::default() then immediately overwrites
config.channels_config with ChannelsConfig::default(), which is redundant
because Config::default() already sets channels_config; remove the explicit
reassignment line (the assignment to config.channels_config =
crate::openhuman::config::ChannelsConfig::default()) and rely on
Config::default() to provide the default ChannelsConfig in the test setup for
functions using Config.

src/openhuman/agent/multimodal.rs (2)

629-632: Use a tempdir-derived missing path for platform-safe determinism.

Line 629 currently uses a hardcoded absolute path; generating a missing path from tempfile::tempdir() is more reliable across CI environments.

Refactor suggestion

-        let err = normalize_local_image("/definitely/missing.png", 1024)
+        let temp = tempfile::tempdir().unwrap();
+        let missing = temp.path().join("missing.png");
+        let err = normalize_local_image(missing.to_string_lossy().as_ref(), 1024)
             .await
             .expect_err("missing local file should fail");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/agent/multimodal.rs` around lines 629 - 632, The test uses a
hardcoded absolute path; replace it with a path derived from a
tempfile::tempdir() so the "missing" file is platform-safe and deterministic:
create a TempDir via tempfile::tempdir(), join a non-existent filename (e.g.,
temp_dir.path().join("definitely_missing.png")) and pass that path to
normalize_local_image(...) in the test; keep the expect_err assertion and the
check that err.to_string() contains "not found or unreadable" so the intent of
normalize_local_image(...) is preserved.

---

612-614: Avoid exact full error-string equality in tests.

Line 612-Line 614 asserts the entire formatted error text, which is brittle to harmless wording changes.

Refactor suggestion

-        assert_eq!(
-            validate_mime("x", "text/plain").unwrap_err().to_string(),
-            "multimodal image MIME type is not allowed for 'x': text/plain"
-        );
+        let err = validate_mime("x", "text/plain").unwrap_err().to_string();
+        assert!(err.contains("MIME type is not allowed"));
+        assert!(err.contains("text/plain"));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/agent/multimodal.rs` around lines 612 - 614, The test currently
asserts exact equality of the formatted error string from validate_mime("x",
"text/plain"), which is brittle; change the assertion to check for a stable
substring or error kind instead — for example call let err = validate_mime("x",
"text/plain").unwrap_err().to_string(); then assert!(err.contains("multimodal
image MIME type is not allowed") && err.contains("'x'")) or, if the error type
is exposed, match on the error variant from validate_mime instead of comparing
the full Display output; this keeps the test robust while still verifying the
expected failure.

src/openhuman/context/debug_dump.rs (1)

754-785: Consider using RAII for test workspace cleanup.

Several tests create temporary workspaces but use let _ = std::fs::remove_dir_all(workspace); for cleanup, which won't execute if the test panics. Consider using the tempfile crate's TempDir for automatic cleanup, or at minimum document that manual cleanup may leave directories on panic.

♻️ Example with tempfile crate

+use tempfile::TempDir;
+
 #[test]
 fn render_main_agent_dump_includes_tool_instructions_and_skill_count() {
-    let workspace =
-        std::env::temp_dir().join(format!("openhuman_debug_main_{}", uuid::Uuid::new_v4()));
-    std::fs::create_dir_all(&workspace).unwrap();
+    let temp_dir = TempDir::new().unwrap();
+    let workspace = temp_dir.path();
     std::fs::write(workspace.join("SOUL.md"), "# Soul\nContext").unwrap();
     // ... rest of test
-    let _ = std::fs::remove_dir_all(workspace);
+    // TempDir automatically cleans up on drop
 }

Also applies to: 817-858, 860-902, 904-961

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/context/debug_dump.rs` around lines 754 - 785, Tests like
render_main_agent_dump_includes_tool_instructions_and_skill_count create
temporary workspace dirs and call std::fs::remove_dir_all at the end, which
won't run on panic; update these tests to use tempfile::TempDir (or similar RAII
temp-dir helper) so the directory is created via TempDir::new() and files are
written to temp_dir.path(), removing the manual std::fs::remove_dir_all calls;
apply this change to the test above and the other tests referencing the same
pattern (those that call render_main_agent_dump, use StubTool and ToolCategory,
and manually remove the workspace).

src/openhuman/agent/bus.rs (1)

30-41: Remove duplicated rustdoc block for AgentTurnRequest.

The same doc text appears twice consecutively, which adds noise and makes maintenance harder.

✂️ Suggested cleanup

-/// Full owned payload for a single agentic turn executed through the bus.
-///
-/// All fields are either owned values, [`Arc`]s, or channel handles — the
-/// bus carries them by value without touching serialization. Consumers can
-/// therefore pass trait objects (`Arc<dyn Provider>`, tool trait-object
-/// registries) and streaming senders (`on_delta`) through unchanged.
 /// Full owned payload for a single agentic turn executed through the bus.
 ///
 /// All fields are either owned values, [`Arc`]s, or channel handles — the
 /// bus carries them by value without touching serialization. Consumers can
 /// therefore pass trait objects (`Arc<dyn Provider>`, tool trait-object
 /// registries) and streaming senders (`on_delta`) through unchanged.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/agent/bus.rs` around lines 30 - 41, The rustdoc comment for
AgentTurnRequest is duplicated; remove the redundant copy so only a single doc
block precedes the AgentTurnRequest definition, ensuring the remaining doc text
is unchanged and whitespace/formatting around the struct/type declaration is
preserved.

src/openhuman/agent/harness/session/runtime.rs (1)

491-492: Avoid fixed sleep for async event assertions.

At Line 491, a fixed 20ms delay can be flaky under CI load. Prefer waiting on a bounded condition (timeout + loop) before asserting captured events.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/agent/harness/session/runtime.rs` around lines 491 - 492,
Replace the fixed sleep+lock with a bounded wait loop using
tokio::time::timeout: repeatedly acquire the events Mutex (events.lock().await)
and check the desired condition on captured (e.g., non-empty or expected length)
until it is satisfied or the timeout elapses, returning a test failure or panic
when timeout occurs; use tokio::time::Duration for the overall timeout and a
short async yield (or await on the Mutex again) inside the loop to avoid
busy-waiting so the assertion is robust under CI.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/openhuman/agent/memory_loader.rs (1)

226-230: ⚠️ Potential issue | 🟡 Minor

Rename this test to match the asserted behavior.

Line 226 says “zero budget disables output”, but Line 229 asserts output is present. This makes the test intent misleading.

Proposed rename

-    async fn new_enforces_minimum_limit_and_zero_budget_disables_output() {
+    async fn new_enforces_minimum_limit_and_zero_budget_is_unlimited() {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/agent/memory_loader.rs` around lines 226 - 230, The test
function new_enforces_minimum_limit_and_zero_budget_disables_output is misnamed
because it asserts that output is present; rename it to reflect the asserted
behavior (e.g., new_enforces_minimum_limit_and_zero_budget_allows_output) and
keep the existing test body that constructs DefaultMemoryLoader::new(0,
0.4).with_max_chars(0), calls load_context(&MockMemory, "hello"), and asserts
context.contains("[Memory context]").

src/openhuman/agent/triage/escalation.rs (1)

187-187: ⚠️ Potential issue | 🟡 Minor

Replace fixed sleeps with condition-based waits

These sleeps are timing-dependent and can intermittently fail in CI under load. Please wait on a concrete predicate under timeout instead.

🛠️ Deterministic wait pattern

-    use tokio::time::{sleep, Duration};
+    use tokio::time::{timeout, Duration};

@@
-        sleep(Duration::from_millis(20)).await;
+        timeout(Duration::from_secs(1), async {
+            loop {
+                if seen.lock().await.iter().any(|event| matches!(
+                    event,
+                    DomainEvent::TriggerEvaluated { decision, external_id, .. }
+                        if decision == "drop" && external_id == "esc-1"
+                )) {
+                    break;
+                }
+                tokio::task::yield_now().await;
+            }
+        })
+        .await
+        .expect("timed out waiting for evaluated event");

Also applies to: 243-243, 278-278, 321-321, 362-362

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/agent/triage/escalation.rs` at line 187, The code imports and
uses fixed sleeps (tokio::time::sleep, Duration) in escalation.rs which makes
tests flaky; replace those sleeps with a condition-based wait under a timeout.
Add a small helper (e.g., wait_for_condition or wait_for_predicate) that accepts
an async predicate, a total timeout, and a poll interval, and implement it using
tokio::time::timeout(...) around a loop that checks the predicate and awaits a
short sleep between checks; then replace direct sleep(...) calls in the
functions referencing escalation logic (look for uses of sleep and Duration in
escalation.rs) with calls to this helper so tests wait for the concrete
predicate rather than a fixed delay. Ensure the helper returns a clear
Result/timeout error so callers can handle failures.

src/openhuman/agent/harness/session/turn.rs (2)

1171-1179: Guard scripted provider queue exhaustion to avoid panic-based test failures.

remove(0) panics when the scripted response queue is empty, which makes failures less diagnosable than returning a typed error.

Proposed fix

         async fn chat(
             &self,
             request: ChatRequest<'_>,
             _model: &str,
             _temperature: f64,
         ) -> Result<ChatResponse> {
             self.requests.lock().await.push(request.messages.to_vec());
-            self.responses.lock().await.remove(0)
+            let mut responses = self.responses.lock().await;
+            if responses.is_empty() {
+                return Err(anyhow::anyhow!(
+                    "SequenceProvider response script exhausted"
+                ));
+            }
+            responses.remove(0)
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/agent/harness/session/turn.rs` around lines 1171 - 1179, The
chat method currently does self.responses.lock().await.remove(0) which will
panic if the scripted response queue is empty; change it to guard against empty
responses and return a typed error instead. Inside chat (function name chat,
types ChatRequest/ChatResponse, fields requests/responses), after pushing the
request, acquire the responses lock and check if responses.is_empty(); if empty,
return an Err with a clear error (e.g., an anyhow/eyre or crate-specific error)
indicating the scripted response queue is exhausted; otherwise pop or remove the
first element and return it as Ok. Ensure no unwrap/remove(0) is left to avoid
panic-based test failures.

---

1124-1124: Use a bounded wait instead of fixed sleep for async event assertions.

The fixed 20ms delay can be flaky under load. Prefer waiting until expected events arrive (with timeout).

Proposed fix

-    use tokio::time::{sleep, timeout, Duration};
+    use tokio::time::{timeout, Duration};
@@
-        sleep(Duration::from_millis(20)).await;
+        timeout(Duration::from_secs(1), async {
+            loop {
+                if events.lock().await.len() >= 2 {
+                    break;
+                }
+                tokio::task::yield_now().await;
+            }
+        })
+        .await
+        .expect("expected ToolExecutionStarted and ToolExecutionCompleted events");
         let captured = events.lock().await;

Also applies to: 1453-1455

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/agent/harness/session/turn.rs` at line 1124, Replace the fixed
sleep(Duration::from_millis(20)).await used to wait for async events with a
bounded wait that uses tokio::time::timeout and actively awaits the expected
event or condition; specifically, where you currently call sleep(...) use
timeout(Duration::from_millis(<sane-ms>), async { loop { if
check_event_or_stream_has_expected_item() { break } yield to the executor (e.g.,
tokio::task::yield_now().await) } }).await and handle the Err(timeout) case as a
test failure; apply this change for all occurrences that import or use sleep,
timeout, Duration (including the spots referenced by the review: the usage at
the import line and the other occurrences around the second instance).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/openhuman/agent/triage/escalation.rs (1)

244-245: ⚠️ Potential issue | 🟡 Minor

Replace timing-based sleeps with condition-based waits in async tests.

Using fixed sleep(Duration::from_millis(20)) makes these tests scheduler-sensitive and flaky under slower CI.

Suggested deterministic wait pattern

-    use tokio::time::{sleep, Duration};
+    use tokio::time::{timeout, Duration};

@@
-        sleep(Duration::from_millis(20)).await;
+        timeout(Duration::from_secs(1), async {
+            loop {
+                if seen.lock().await.iter().any(|event| matches!(
+                    event,
+                    DomainEvent::TriggerEvaluated { decision, external_id, .. }
+                        if decision == "drop" && external_id == "esc-drop"
+                )) {
+                    break;
+                }
+                tokio::task::yield_now().await;
+            }
+        })
+        .await
+        .expect("timed out waiting for evaluated event");

Also applies to: 280-281, 324-325, 366-367

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/agent/triage/escalation.rs` around lines 244 - 245, Replace the
flaky sleep(Duration::from_millis(20)).await calls with deterministic
condition-based waits: stop using the literal sleep and instead await a concrete
signal (e.g., a tokio::sync::Notify, oneshot channel, or poll a shared
atomic/flag inside a tokio::time::timeout loop) that tests can trigger when the
expected state/event occurs; specifically, replace each occurrence of
sleep(Duration::from_millis(20)).await (and the other similar sleeps at the
noted locations) with code that uses tokio::time::timeout to bound waiting and
either await notify.notified().await or repeatedly check the condition in a
short interval until satisfied, failing the test on timeout to avoid
scheduler-dependent flakes.

src/openhuman/local_ai/mod.rs (1)

3-5: Add a short rustdoc for the test mutex’s intent.

This global test lock coordinates cross-module shared-state mutation; a brief comment will make that coupling explicit.

Suggested tweak

+/// Test-only global lock to serialize tests that mutate shared Local AI service state.
 #[cfg(test)]
 pub(crate) static LOCAL_AI_TEST_MUTEX: once_cell::sync::Lazy<std::sync::Mutex<()>> =
     once_cell::sync::Lazy::new(|| std::sync::Mutex::new(()));

As per coding guidelines "{src/**/*.rs,.github/**/*.md,docs/**/*.md}: Add concise rustdoc / code comments where the flow is not obvious".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/local_ai/mod.rs` around lines 3 - 5, Add a concise rustdoc
above the static LOCAL_AI_TEST_MUTEX explaining that it is a test-only global
mutex used to coordinate and serialize cross-module shared-state mutation in
tests; mention that it exists to avoid test flakiness by preventing concurrent
access to the shared resources and that it is only compiled under #[cfg(test)].
Update the comment to live immediately above the declaration of
LOCAL_AI_TEST_MUTEX so readers of the mod.rs can immediately see the intent and
scope.

src/openhuman/voice/postprocess.rs (1)

179-184: Make shared-state restoration panic-safe in this test.

If the async call panics, the previous state is never restored, which can leak state into later tests.

Panic-safe restore pattern

-        let result = rt.block_on(cleanup_transcription(&config, "um hello uh world", None));
-        service.status.lock().state = previous;
+        let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
+            rt.block_on(cleanup_transcription(&config, "um hello uh world", None))
+        }));
+        service.status.lock().state = previous;
+        let result = result.expect("cleanup_transcription panicked");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/voice/postprocess.rs` around lines 179 - 184, The test
currently saves previous = service.status.lock().state and then resets it after
calling rt.block_on, which leaks state if the async call panics; make
restoration panic-safe by creating a RAII guard that stores a reference to
service (local_ai::global(&config) result) and previous state and restores
service.status.lock().state in its Drop impl, construct that guard immediately
after computing previous (e.g., let _restore = RestoreGuard::new(&service,
previous)), then call rt.block_on(cleanup_transcription(...)); the guard's Drop
will always run and restore the state even if the call panics.

tests/agent_memory_loader_public.rs (1)

24-35: Use limit in ScriptedMemory::recall to keep the test double faithful.

At Line 27, _limit is ignored, so these tests can’t catch regressions where DefaultMemoryLoader passes a wrong recall limit.

♻️ Proposed refinement

-    async fn recall(
-        &self,
-        query: &str,
-        _limit: usize,
-        _session_id: Option<&str>,
-    ) -> Result<Vec<MemoryEntry>> {
-        if query.contains("working.user") {
-            Ok(self.working.clone())
-        } else {
-            Ok(self.primary.clone())
-        }
-    }
+    async fn recall(
+        &self,
+        query: &str,
+        limit: usize,
+        _session_id: Option<&str>,
+    ) -> Result<Vec<MemoryEntry>> {
+        let source = if query.contains("working.user") {
+            &self.working
+        } else {
+            &self.primary
+        };
+        Ok(source.iter().take(limit).cloned().collect())
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/agent_memory_loader_public.rs` around lines 24 - 35, The test double's
recall method ignores the _limit parameter; update ScriptedMemory::recall (the
async fn recall) to respect the provided limit by returning at most limit
entries from either self.working or self.primary (preserve original ordering),
and handle limit == 0 or limit > len gracefully; ensure the method uses the
limit argument (not _limit) so tests can detect incorrect recall limits from
DefaultMemoryLoader.

tests/agent_builder_public.rs (1)

12-116: Consider centralizing repeated test doubles into shared test utilities.

StubProvider/StubMemory are duplicated across public test files, which can drift when trait contracts change.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/agent_builder_public.rs` around lines 12 - 116, The StubProvider,
StubTool, and StubMemory test doubles are duplicated and should be centralized:
extract these structs and their async_trait impls
(StubProvider::chat_with_system, ::chat;
StubTool::name/description/parameters_schema/execute;
StubMemory::store/recall/get/list/forget/count/health_check/name) into a single
shared test support module (e.g., tests::support or tests::utils) and re-export
them for use by public tests; update test files to import the shared module
instead of redefining the doubles and ensure the module exposes any needed trait
imports (Provider, Tool, Memory, MemoryCategory, ChatRequest, ChatResponse,
ToolResult, MemoryEntry) and async_trait usage so trait contract changes only
need one update.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/openhuman/memory/embeddings.rs (1)

123-163: Add debug/trace logs for ORT dylib path branch decisions.

This changed resolution flow would benefit from branch-level diagnostics (selected source, candidate path, fallback used) to make runtime failures easier to triage.

As per coding guidelines: src/**/*.rs: "Add substantial, development-oriented logs on new/changed flows; include logs at ... branch decisions ... and error handling paths."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/openhuman/memory/embeddings.rs` around lines 123 - 163, The function
ensure_fastembed_ort_dylib_path lacks diagnostic logging for its branch
decisions; add development-level logs (using the project's logger or tracing
crate) that record: entry into ensure_fastembed_ort_dylib_path and current
ORT_DYLIB_PATH if set, when ORT_LIB_LOCATION is present and the inspected
candidate path (and whether it was a file), which platform runtime_lib path was
checked and whether it existed, and which system fallback candidate (from the
Linux array) was selected or if none matched; instrument checks around
env::var_os("ORT_DYLIB_PATH"), env::var_os("ORT_LIB_LOCATION"), the
candidate.is_file() branch, the runtime_lib.exists() branch, and the Linux
fallback loop so each branch decision and final action (setting ORT_DYLIB_PATH
or leaving it unset) is logged.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes